Exclude expired transactions when counting block size during block creation #13850

vusirikala · 2024-06-27T21:29:03Z

Description

Exclude expired transactions when counting block size during block creation

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

bchocho · 2024-07-10T07:33:14Z

consensus/src/quorum_store/utils.rs

+                                                // latest_block_timestamp is microseonds since UNIX epoch
+                                                // expiration_timestamp_secs is seconds since UNIX epoch
+                                                // giving a second buffer for expiration as the expiration time is rounded off to seconds 
+                                            && ((txn_summary.expiration_timestamp_secs + 1) * MICRO_SEC_PER_SEC)


We should be careful here.

The multiplication as-is could overflow. So we should instead do integer division.

We should match the move spec for expiration.

The move spec appears to be

aptos-core/aptos-move/framework/aptos-framework/sources/transaction_validation.move

Lines 83 to 86 in 533f3c3

assert!(

timestamp::now_seconds() < txn_expiration_time,

error::invalid_argument(PROLOGUE_ETRANSACTION_EXPIRED),

);

where now_seconds() is the current block timestamp truncated to seconds.

So it appears the correct condition is:
(self.latest_block_timestamp + 1) / MICRO_SEC_PER_SEC < txn_summary.expiration_timestamp_secs
because the current block timestamp must be monotonically increasing from the previous timestamp.

If we want a truly accurate expiration condition, we can pass in the current block's timestamp from proposal_generator. I'm not sure that's necessary, as the number of truly unexpired transactions <= unique txn size based on the txn summaries we have.

If you are intending to approximate self.latest_block_timestamp/MICRO_SEC_PER_SEC with the next integer value, did you mean (self.latest_block_timestamp + MICRO_SEC_PER_SEC) / MICRO_SEC_PER_SEC < txn_summary.expiration_timestamp_secs?

sitalkedia

LG

github-actions · 2024-07-10T22:17:12Z

✅ Forge suite `compat` success on `1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5` ==> `6bb88b7780ba238488dbb01c53a50e79333aaebd`

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 6bb88b7780ba238488dbb01c53a50e79333aaebd (PR)
1. Check liveness of validators at old version: 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5
compatibility::simple-validator-upgrade::liveness-check : committed: 10516.774183309159 txn/s, latency: 3224.6912988578315 ms, (p50: 3000 ms, p90: 5600 ms, p99: 7100 ms), latency samples: 348460
2. Upgrading first Validator to new version: 6bb88b7780ba238488dbb01c53a50e79333aaebd
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7287.3713250447045 txn/s, latency: 3635.6935646415204 ms, (p50: 4000 ms, p90: 4400 ms, p99: 4500 ms), latency samples: 138920
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6385.313187047027 txn/s, latency: 4858.307865755627 ms, (p50: 4700 ms, p90: 5500 ms, p99: 8600 ms), latency samples: 248800
3. Upgrading rest of first batch to new version: 6bb88b7780ba238488dbb01c53a50e79333aaebd
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7550.922100173426 txn/s, latency: 3542.0749362606234 ms, (p50: 3900 ms, p90: 4300 ms, p99: 4400 ms), latency samples: 141200
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6562.282723081964 txn/s, latency: 4655.595098279096 ms, (p50: 4400 ms, p90: 7600 ms, p99: 8300 ms), latency samples: 245220
4. upgrading second batch to new version: 6bb88b7780ba238488dbb01c53a50e79333aaebd
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 12140.01126081186 txn/s, latency: 2245.1682454150123 ms, (p50: 2500 ms, p90: 2600 ms, p99: 2800 ms), latency samples: 211560
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 11623.031669787533 txn/s, latency: 2741.580537857067 ms, (p50: 2700 ms, p90: 3700 ms, p99: 5600 ms), latency samples: 376680
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 6bb88b7780ba238488dbb01c53a50e79333aaebd passed
Test Ok

github-actions · 2024-07-10T22:18:38Z

✅ Forge suite `realistic_env_max_load` success on `6bb88b7780ba238488dbb01c53a50e79333aaebd`

two traffics test: inner traffic : committed: 9104.509651448549 txn/s, latency: 4373.826951053234 ms, (p50: 4200 ms, p90: 5100 ms, p99: 11500 ms), latency samples: 3461720
two traffics test : committed: 100.0335028302447 txn/s, latency: 2229.1155555555556 ms, (p50: 2100 ms, p90: 2500 ms, p99: 6500 ms), latency samples: 1800
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.249, avg: 0.227", "QsPosToProposal: max: 1.931, avg: 1.707", "ConsensusProposalToOrdered: max: 0.323, avg: 0.297", "ConsensusOrderedToCommit: max: 0.415, avg: 0.397", "ConsensusProposalToCommit: max: 0.718, avg: 0.694"]
Max round gap was 1 [limit 4] at version 1674246. Max no progress secs was 5.741337 [limit 15] at version 1674246.
Test Ok

github-actions · 2024-07-10T22:21:19Z

✅ Forge suite `framework_upgrade` success on `1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5` ==> `6bb88b7780ba238488dbb01c53a50e79333aaebd`

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 6bb88b7780ba238488dbb01c53a50e79333aaebd (PR)
Upgrade the nodes to version: 6bb88b7780ba238488dbb01c53a50e79333aaebd
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1232.110165438944 txn/s, submitted: 1235.3623415920633 txn/s, failed submission: 3.2521761531193847 txn/s, expired: 3.2521761531193847 txn/s, latency: 2707.3862556561085 ms, (p50: 2100 ms, p90: 4800 ms, p99: 9600 ms), latency samples: 106080
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1189.0205780541537 txn/s, submitted: 1192.8758803289813 txn/s, failed submission: 3.855302274827506 txn/s, expired: 3.855302274827506 txn/s, latency: 2568.7010871638377 ms, (p50: 1900 ms, p90: 4800 ms, p99: 9700 ms), latency samples: 104860
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 6bb88b7780ba238488dbb01c53a50e79333aaebd passed
Upgrade the remaining nodes to version: 6bb88b7780ba238488dbb01c53a50e79333aaebd
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1209.3412312239197 txn/s, submitted: 1212.5563427387574 txn/s, failed submission: 3.2151115148376146 txn/s, expired: 3.2151115148376146 txn/s, latency: 2687.4405526015953 ms, (p50: 1800 ms, p90: 5100 ms, p99: 10500 ms), latency samples: 105320
Test Ok

…eation (#13850) * Use proof queue asynchronously * Committing what I have * Sending AddBatches message * Calcuating the remaining txns * Calculate proof queue size correctly * Add a counter * Update pfn_const_tps test * Minor changes * Minor change * Add some coutners * Rust lint * Increasing quorum store backpressure limits * setting dynamic_min_txns_per_sec to 160 * Fixing the calculation * increase vfns to 7 * Fixing the typo in batch generator * Add increase fraction * Removing skipped transactions after inserting them * Add some counters * Update consensus pending duration counter * Add more counters * Increasing block size to 2500 * Update a counter * Increase block size limit * Resetting execution config params * Moving proof queue to utils.rs * Moving counters * Use transaction summary * intelligent pull proofs * Fix a bug in pull proofs * Fix the bug * Rest to full to false in every iteration * Addressing PR comments * Move backpressure_tx to proof queue * Add info statement * Change buckets * Add some info statements * Cleanup * Remove an unrelated change * Addressing PR comments * Addressing PR comments * Add some timer counters * Add more timer counters * Minor optimization * Proof queue to be part of proof manager * Move some code to a function * Minor fixes * Add max_unique_txns parameter * Use Lazy * Removing comments * Minor change * Minor change * Minor fix * Add unit test and address PR comments * Minor fix in proof manager * Use saturating_sub * Exclude expired transactions when counting block size * Minor fix * Addressing PR comments * Minor fix * Change the expiration units * Fixing unit tests * Update unit tests * renaming * Add block_timestamp as inputt to pull_proofs * Fix test

vusirikala added 30 commits June 2, 2024 19:14

Use proof queue asynchronously

60e8b87

Committing what I have

b56cc33

Sending AddBatches message

a369c28

Calcuating the remaining txns

80a5a34

Calculate proof queue size correctly

4e5babe

Add a counter

1653475

Update pfn_const_tps test

844e4ae

Minor changes

be89d17

Minor change

8d77c78

Add some coutners

e064652

Rust lint

204d823

Increasing quorum store backpressure limits

32eb5a4

setting dynamic_min_txns_per_sec to 160

7f63400

Merge branch 'main' into satya/proof_queue

196412f

Fixing the calculation

76f9326

increase vfns to 7

4c7af58

Fixing the typo in batch generator

f71d2d0

Merge branch 'main' into satya/proof_queue

2ee53a8

Merge branch 'main' into satya/proof_queue

30490c3

Merge branch 'main' into satya/proof_queue

e1dcb03

Merge branch 'main' into satya/proof_queue

6282c44

Add increase fraction

29951a9

Removing skipped transactions after inserting them

cc3d7a2

Add some counters

5319f79

Update consensus pending duration counter

a4f11ab

Add more counters

eabe96c

Increasing block size to 2500

0c413fd

Update a counter

78b559b

Increase block size limit

056baec

Resetting execution config params

6e54abb

bchocho reviewed Jul 10, 2024

View reviewed changes

Add block_timestamp as inputt to pull_proofs

8ab96f4

This comment has been minimized.

Sign in to view

bchocho approved these changes Jul 10, 2024

View reviewed changes

Fix test

0a93374

vusirikala requested review from igor-aptos and removed request for ibalajiarun July 10, 2024 20:42

igor-aptos approved these changes Jul 10, 2024

View reviewed changes

vusirikala enabled auto-merge (squash) July 10, 2024 20:55

sitalkedia approved these changes Jul 10, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

vusirikala added 2 commits July 10, 2024 16:45

Merge branch 'main' into satya/count_unexpired

45148be

Typo

6bb88b7

This comment has been minimized.

Sign in to view

vusirikala merged commit f1eac9b into main Jul 10, 2024
47 checks passed

vusirikala deleted the satya/count_unexpired branch July 10, 2024 22:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude expired transactions when counting block size during block creation #13850

Exclude expired transactions when counting block size during block creation #13850

vusirikala commented Jun 27, 2024 •

edited

Loading

bchocho Jul 10, 2024

vusirikala Jul 10, 2024

This comment has been minimized.

This comment has been minimized.

sitalkedia left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jul 10, 2024

github-actions bot commented Jul 10, 2024

github-actions bot commented Jul 10, 2024

	assert!(
	timestamp::now_seconds() < txn_expiration_time,
	error::invalid_argument(PROLOGUE_ETRANSACTION_EXPIRED),
	);

Exclude expired transactions when counting block size during block creation #13850

Exclude expired transactions when counting block size during block creation #13850

Conversation

vusirikala commented Jun 27, 2024 • edited Loading

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

bchocho Jul 10, 2024

Choose a reason for hiding this comment

vusirikala Jul 10, 2024

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

sitalkedia left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jul 10, 2024

✅ Forge suite compat success on 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 6bb88b7780ba238488dbb01c53a50e79333aaebd

github-actions bot commented Jul 10, 2024

✅ Forge suite realistic_env_max_load success on 6bb88b7780ba238488dbb01c53a50e79333aaebd

github-actions bot commented Jul 10, 2024

✅ Forge suite framework_upgrade success on 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 6bb88b7780ba238488dbb01c53a50e79333aaebd

vusirikala commented Jun 27, 2024 •

edited

Loading

✅ Forge suite `compat` success on `1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5` ==> `6bb88b7780ba238488dbb01c53a50e79333aaebd`

✅ Forge suite `realistic_env_max_load` success on `6bb88b7780ba238488dbb01c53a50e79333aaebd`

✅ Forge suite `framework_upgrade` success on `1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5` ==> `6bb88b7780ba238488dbb01c53a50e79333aaebd`